Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Emergency migration to jwt-go v4 #107

Closed
wants to merge 1 commit into from
Closed

Conversation

GSokol
Copy link

@GSokol GSokol commented Apr 30, 2020

To fix jwt parsing problem (issue in jwt-go #348) emergency migrated to v4.

To fix jwt parsing problem (issue in jwt-go [centrifugal#348](dgrijalva/jwt-go#348)) emergency migrated to v4.
@FZambia
Copy link
Member

FZambia commented Apr 30, 2020

@GSokol hello!

Could you explain why this is an emergency migration? I think that most of Centrifuge and Centrifugo users generate JWT themselves so simply not including audience as array makes things work. What's your case? Do you have JWT that comes from external provider so you can't change its content? Or there is some other reason?

Since this pull request introduces migration to preview version of JWT library it requires a careful consideration on what is the best way to deal with this problem.

@GSokol
Copy link
Author

GSokol commented May 1, 2020

@FZambia, hello!

Sorry for panic! In my case tokens are issued by third party auth service. I used Microsoft Authentication Server, then Ory Hydra. Yes, maybe, that’s not generally emergency situation, but standard discomplaints seems confusing.

@FZambia
Copy link
Member

FZambia commented May 1, 2020

Will try to look at jwt-go alternatives. For example https://github.com/cristalhq/jwt seems very clean and performs a bit better for decoding.

Btw are you using Centrifugo server or this library directly?

@GSokol
Copy link
Author

GSokol commented May 1, 2020

Yes, I'm using Centrifugo server. Why don't you want to use https://github.com/square/go-jose implementation?

@FZambia
Copy link
Member

FZambia commented May 2, 2020

Why don't you want to use https://github.com/square/go-jose implementation?

Just a personal preference I suppose, go-jose has a bit more than we need here at moment (JWS, JWE). https://github.com/cristalhq/jwt has flexible API that allows to tune behaviour from outside - i.e. feels like a constructor. It does not use interface{} types which is a good thing. It is fuzzy tested. Also I know an author of library so in case of any problems it is simple to quickly collaborate for a fix. Actually we have already found some things to be improved when experimenting with this migration.

@Skarm
Copy link
Contributor

Skarm commented May 2, 2020

jwt implementation a lot, for example another https://github.com/pascaldekloe/jwt

@cristaloleg
Copy link
Member

@FZambia thanks (cristalhq/jwt author here). Yep, I've tried to make it as error-prone as possible, due to collaboration with @FZambia we've found few places that can be improved and they will be fixed soon (also some doc fixes).

Maybe library isn't so popular, but it does what it should. Suggestions are pretty welcome.

@FZambia
Copy link
Member

FZambia commented May 8, 2020

Closing in favour of #109 which was just merged. @GSokol thanks for report, I think your case must be solved with next server release. If you want - you can post an example token in your format and I'll double check it is being decoded well now (I added a couple of tests for tokens with audience inside that pr though).

@FZambia FZambia closed this May 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants